Skip to content

fix(review): inspect complete file lists for large pull requests - #984

Draft
vincentkoc wants to merge 2 commits into
mainfrom
fix/complete-pull-files
Draft

fix(review): inspect complete file lists for large pull requests#984
vincentkoc wants to merge 2 commits into
mainfrom
fix/complete-pull-files

Conversation

@vincentkoc

@vincentkoc vincentkoc commented Jul 31, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Related: #983

What Problem This Solves

Fixes an issue where large OpenClaw pull requests could hide config or persistent-data files in the omitted middle of the bounded 80-file review context, leaving deterministic merge-risk classification incomplete.

Why This Change Was Made

For truncated openclaw/openclaw pull requests only, ClawSweeper now fetches the complete file list into a compact non-enumerable sidecar. It reuses every bounded-context page, requires the exact count and unique filenames, verifies that base and head SHAs stayed stable across pagination, and fails closed above GitHub's 3,000-file response limit. Deterministic config and data-model classifiers consume that sidecar, while prompts, ledgers, and cache digests keep the existing bounded context.

User Impact

Large OpenClaw pull requests receive complete deterministic surface classification without increasing model prompt size or changing review cache identity. Fetch or count failures retain the existing unknown-truncated fail-closed marker.

Evidence

  • Direct TypeScript build passed.
  • Focused context, surface-policy, and prompt-context tests: 61/61 passed.
  • Fresh uncommitted and committed-branch autoreview: no accepted or actionable findings; final exact-branch confidence 0.91.
  • git diff --check: clean.
  • GitHub's current List pull requests files contract caps responses at 3,000 files: https://docs.github.com/en/rest/pulls/pulls#list-pull-requests-files. Totals above that limit fail closed before complete pagination.
  • The ClawSweeper changelog entry is intentional: this PR changes ClawSweeper itself. The current origin/main release-owned exception applies to OpenClaw's changelog during foreign-target PR work, not this repository's own operational release notes.

Real Behavior Proof

  • Claim: a truncated 118-file pull request supplies all files to deterministic classifiers without exposing the hidden payload to model context or cache identity.
  • Exercised surface: live GitHub pull-file pagination for fix(agents): improve Code Mode reliability on smaller models openclaw#115869, complete-count validation, deterministic config/data classifier input, prompt ledger, and content digest.
  • Observed result: live pagination returned exactly 118 files; the sidecar accepted all 118, the config classifier reported no config change, and no unknown-truncated marker remained. The page cache fetched pages [1,2] exactly once rather than refetching both pages. The semantic false-positive cleanup is independently reviewable in fix(review): avoid false persistent data-model findings #983.
  • Failure proof: count mismatch, duplicate filenames, changed base/head snapshots, paginated-fetch exceptions, and totals above GitHub's 3,000-file response limit return no sidecar, preserving unknown-truncated-pull-files.
  • Limits: production review output reflects both this complete-file input fix and the separately reviewed classifier semantics fix after those drafts are landed.

Live Pagination And Merge-Order Proof - July 31, 2026

Redacted live pagination transcript for OpenClaw PR 115869:

captured_at=2026-07-31T21:02:46Z
review_head=4a434e7e1fc530785a6700e088bdbb93b04091a6
page_1_count=100
page_2_count=18
page_3_count=0
total=118
first=docs/providers/huggingface.md
last=src/commands/agent-exec.ts

The transcript was produced with authenticated read-only ghx api requests to:

repos/openclaw/openclaw/pulls/115869/files?per_page=100&page=1
repos/openclaw/openclaw/pulls/115869/files?per_page=100&page=2
repos/openclaw/openclaw/pulls/115869/files?per_page=100&page=3

Both exact merge orders were tested from base 16f01b29508632ab3a904087191e39b20968ab68:

  • 983 -> 984: clean cherry-picks; 66/66 focused tests passed.
  • 984 -> 983: clean cherry-picks; 66/66 focused tests passed.
  • Both orders produced identical Git tree 609478921e5b04e2b1844001bfe81ca713f4f099, with 6 files changed, 560 insertions, and 25 deletions.
  • git diff --check 16f01b29508632ab3a904087191e39b20968ab68..HEAD passed in both worktrees.
  • Both orders produced { "config": { "change": false, "keys": [] }, "data": { "change": false, "surfaces": [] } } over the live 118-file payload.
  • The same suite passed count-mismatch, duplicate-filename, snapshot-drift, fetch-exception, and over-3,000-file cases, which preserve the conservative unknown-truncated-pull-files fallback.
  • Prompt, ledger, and content-digest tests confirmed the complete sidecar remains outside model context and cache identity.

Commands used direct tsc plus node --test test/context.test.ts test/pr-surface-policy.test.ts test/review-prompt-context.test.ts; no dependency reconciliation occurred in the proof worktrees.

Maintainer Decision - July 31, 2026

Approved: accept the bounded pagination cost. For a truncated OpenClaw pull request at GitHub's 3,000-file ceiling, the complete classifier path performs at most 30 file-list page reads plus one refreshed pull snapshot. Already-read bounded-context pages are reused, non-OpenClaw targets do not enter this path, and totals above 3,000 or any count, uniqueness, fetch, or source-snapshot mismatch fail closed to the existing unknown-truncated marker. This trade is acceptable for deterministic config and stored-data classification.

@vincentkoc vincentkoc mentioned this pull request Jul 31, 2026
Draft
@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 automation 🚨 Merging this PR could break CI, automerge, proof capture, label sync, or automation. labels Jul 31, 2026
@clawsweeper

clawsweeper Bot commented Jul 31, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs changes before merge. Reviewed August 2, 2026, 9:07 PM ET / August 3, 2026, 01:07 UTC.

ClawSweeper review

What this changes

Adds complete, fail-closed pull-file hydration for truncated OpenClaw pull requests so deterministic configuration and stored-data classifiers can inspect files outside the bounded model context.

Merge readiness

⛔ Blocked by patch quality or review findings - 6 items remain

Keep this PR open for a focused rebase and port. Its live pagination proof is useful, but the branch changes the pre-refactor monolith while current main constructs pull-file context and runs deterministic classification in split modules, so the stated behavior would not reach the active review path after conflict resolution.

Priority: P2
Reviewed head: 4a434e7e1fc530785a6700e088bdbb93b04091a6

Review scores

Measure Result What it means
Overall readiness 🦪 silver shellfish (2/6) The proof for the intended pagination behavior is strong, but the patch is not merge-ready because its central integration point was replaced on current main.
Proof confidence 🦞 diamond lobster (5/6) Sufficient (terminal): The PR body provides a redacted live 118-file pagination transcript plus after-fix failure-mode and merge-order evidence; it must be refreshed after the required port because the current proof targets the old integration path.
Patch quality 🦪 silver shellfish (2/6) 1 actionable review finding remain.

Verification

Check Result Evidence
Real behavior Verified Sufficient (terminal): The PR body provides a redacted live 118-file pagination transcript plus after-fix failure-mode and merge-order evidence; it must be refreshed after the required port because the current proof targets the old integration path.
Evidence reviewed 5 items Current active context path: Current main builds context.pullFiles with an 80-file bounded window in the split item-context module; no complete-file sidecar is attached there.
Current active classifier path: Both deterministic classifiers inspect context.pullFiles and add unknown-truncated-pull-files whenever the bounded file list is truncated.
Branch misses active modules: The PR changes src/clawsweeper.ts and src/clawsweeper-types.ts, but does not modify the current src/clawsweeper-item-context.ts or src/clawsweeper-change-detection.ts paths that own hydration and classifier consumption.
Findings 1 actionable finding [P1] Port the sidecar to the active split modules
Security None None.

How this fits together

ClawSweeper gathers a bounded GitHub pull-file context for model review, then runs deterministic configuration and persisted-data classifiers before emitting merge-risk guidance. The requested sidecar would keep the full file list out of prompts and cache identity while giving those classifiers complete input.

flowchart LR
  A[GitHub pull-file pages] --> B[Bounded review context]
  A --> C[Validated complete-file sidecar]
  B --> D[Model review prompt]
  C --> E[Deterministic surface classifiers]
  E --> F[Merge-risk classification]
  D --> G[Codex review decision]
  F --> G
Loading

Before merge

  • Port the sidecar to the active split modules (P1) - This repeats the prior P1 blocker: the sidecar is attached in the pre-refactor src/clawsweeper.ts context builder, but current main builds pull-file context in src/clawsweeper-item-context.ts and classifies it in src/clawsweeper-change-detection.ts. GitHub already marks the branch dirty; without moving this code and its tests to those active paths, resolving the conflict cannot deliver the advertised complete-file classification.
  • Resolve merge risk (P1) - GitHub reports this draft branch as dirty against current main; resolving that conflict without porting the sidecar into the split context and detection modules would leave truncated reviews on the existing unknown-marker path or regress the refactor.
  • Resolve merge risk (P1) - The change affects automated review classification, so the rebased implementation must preserve bounded prompt content, cache identity, snapshot stability checks, and fail-closed behavior for incomplete pagination.
  • Improve patch quality - Rebase and port the sidecar into the current split context and detection modules.
  • Improve patch quality - Run the focused context, surface-policy, and prompt/cache tests on the rebased head and update the existing live pagination proof.
  • Improve patch quality - Request a fresh ClawSweeper review after the head and PR body evidence are current.

Findings

  • [P1] Port the sidecar to the active split modules — src/clawsweeper.ts:6767-6770
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Patch surface 6 files affected; 400 additions, 10 deletions The implementation is concentrated in the obsolete monolith while the two active split modules remain untouched.
Current review cycles 17 completed cycles The same P1 integration blocker remains unresolved on the unchanged head and should be addressed in one port rather than reviewed again unchanged.

Merge-risk options

Maintainer options:

  1. Port and re-prove the active path (recommended)
    Move hydration and sidecar consumption to the current split modules, retain the bounded prompt/cache boundary and fail-closed guards, then refresh the live or fixture-based proof on the rebased head.
  2. Pause the draft
    Keep the PR open but do not merge the old-monolith implementation if the active-module port cannot be completed as a narrow rebase.
Copy recommended automerge instruction 
@clawsweeper automerge

Special instructions:
Port the complete pull-file sidecar to the current split item-context and change-detection modules; preserve bounded prompt and cache behavior, add focused regression coverage for complete and fail-closed truncated paths, and refresh proof on the rebased head.

Technical review

Best possible solution:

Rebase onto current main, place complete-file hydration in the split item-context path and consumption in the split change-detection path, then prove both complete classification and the existing fail-closed fallback on the rebased head.

Do we have a high-confidence way to reproduce the issue?

Yes, from current source: truncated pull-file context is bounded to 80 entries and both active classifiers add the unknown-truncated marker. The proposed sidecar is attached only in the old monolith, so it cannot reach those current paths.

Is this the best way to solve the issue?

No. The underlying complete-file sidecar design is a reasonable bounded solution, but this branch must port it to src/clawsweeper-item-context.ts and src/clawsweeper-change-detection.ts before it solves the active behavior.

Full review comments:

  • [P1] Port the sidecar to the active split modules — src/clawsweeper.ts:6767-6770
    This repeats the prior P1 blocker: the sidecar is attached in the pre-refactor src/clawsweeper.ts context builder, but current main builds pull-file context in src/clawsweeper-item-context.ts and classifies it in src/clawsweeper-change-detection.ts. GitHub already marks the branch dirty; without moving this code and its tests to those active paths, resolving the conflict cannot deliver the advertised complete-file classification.
    Confidence: 0.99

Overall correctness: patch is incorrect
Overall confidence: 0.99

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against de31c9959070.

Labels

Label justifications:

  • P2: This is a bounded review-automation correctness fix with limited user blast radius, but it is not currently delivering its central behavior on main.
  • merge-risk: 🚨 automation: The PR changes review-context hydration and deterministic risk classification, and its outdated integration point could leave automation on the wrong path after rebase.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦞 diamond lobster and patch quality is 🦪 silver shellfish.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Sufficient (terminal): The PR body provides a redacted live 118-file pagination transcript plus after-fix failure-mode and merge-order evidence; it must be refreshed after the required port because the current proof targets the old integration path.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body provides a redacted live 118-file pagination transcript plus after-fix failure-mode and merge-order evidence; it must be refreshed after the required port because the current proof targets the old integration path.

Evidence

Acceptance criteria:

  • [P1] node --test test/context.test.ts test/pr-surface-policy.test.ts test/review-prompt-context.test.ts.
  • [P1] pnpm run build.
  • [P1] pnpm run check.
  • [P1] Run a redacted live or captured 118-file pagination proof on the rebased head, including complete classification plus count, duplicate, snapshot-drift, fetch-failure, and over-3,000 fail-closed cases.

What I checked:

Likely related people:

  • Peter Steinberger: Introduced the active split context and deterministic-classification modules in the decomposition work that superseded the branch's monolithic integration point. (role: recent area contributor; confidence: high; commits: dccfa528f038, 8fd1140e7aa9; files: src/clawsweeper-item-context.ts, src/clawsweeper-change-detection.ts)

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (17 earlier review cycles; latest 8 shown)
  • reviewed 2026-08-01T17:32:42.402Z sha 4a434e7 :: found issues before merge. :: [P1] Port the sidecar onto the split review modules
  • reviewed 2026-08-01T21:22:55.741Z sha 4a434e7 :: found issues before merge. :: [P1] Port the sidecar onto the split review modules
  • reviewed 2026-08-01T22:38:06.738Z sha 4a434e7 :: found issues before merge. :: [P1] Port the sidecar onto the split review modules
  • reviewed 2026-08-02T08:55:19.646Z sha 4a434e7 :: found issues before merge. :: [P1] Port hydration onto the split review modules
  • reviewed 2026-08-02T12:47:08.651Z sha 4a434e7 :: found issues before merge. :: [P1] Port hydration onto the split review modules
  • reviewed 2026-08-02T15:25:57.239Z sha 4a434e7 :: found issues before merge. :: [P1] Port hydration onto the split review modules
  • reviewed 2026-08-02T17:35:36.060Z sha 4a434e7 :: needs changes before merge. :: [P1] Port the sidecar to the split review modules
  • reviewed 2026-08-02T21:03:50.574Z sha 4a434e7 :: needs changes before merge. :: [P1] Port the sidecar to the split review modules

@vincentkoc

vincentkoc commented Jul 31, 2026

Copy link
Copy Markdown
Member Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 31, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: when the review finishes, ClawSweeper will create the durable review comment if needed or update the existing comment in place.

Re-review progress:

@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. and removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. labels Jul 31, 2026
@vincentkoc

vincentkoc commented Jul 31, 2026

Copy link
Copy Markdown
Member Author

@clawsweeper re-review

@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. labels Jul 31, 2026
@clawsweeper clawsweeper Bot added proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. and removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. labels Jul 31, 2026
@vincentkoc

vincentkoc commented Jul 31, 2026

Copy link
Copy Markdown
Member Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 31, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: when the review finishes, ClawSweeper will create the durable review comment if needed or update the existing comment in place.

Re-review progress:

@vincentkoc

vincentkoc commented Jul 31, 2026

Copy link
Copy Markdown
Member Author

Maintainer decision for head 4a434e7: accept the bounded API budget of at most 30 paginated file-list reads plus one refreshed pull snapshot at the 3,000-file ceiling. Keep stable-snapshot validation, page reuse, OpenClaw-only scope, and fail-closed behavior above the ceiling.

@vincentkoc

vincentkoc commented Jul 31, 2026

Copy link
Copy Markdown
Member Author

@clawsweeper re-review

@vincentkoc vincentkoc mentioned this pull request Jul 31, 2026
Draft
@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. proof: sufficient Contributor real behavior proof is sufficient. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. proof: sufficient Contributor real behavior proof is sufficient. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. labels Aug 1, 2026
@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. proof: sufficient Contributor real behavior proof is sufficient. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. proof: sufficient Contributor real behavior proof is sufficient. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. labels Aug 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

merge-risk: 🚨 automation 🚨 Merging this PR could break CI, automerge, proof capture, label sync, or automation. P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@vincentkoc